add support for QuerySet.annotate() #39
Conversation
timgraham
force-pushed
the
QuerySet.annotate
branch
from
85c4206 to
e4cc20d
Compare
timgraham
force-pushed
the
QuerySet.annotate
branch
from
e4cc20d to
0dd8d6b
Compare
django_mongodb/query.py
Outdated
| cursor.limit(int(self.query.high_mark - self.query.low_mark)) | ||
| return cursor | ||
| pipeline.append({"$limit": self.query.high_mark - self.query.low_mark}) | ||
| return self.collection.aggregate(pipeline) |
There was a problem hiding this comment.
I think I saw this in other PR. Is there any order to follow?
There was a problem hiding this comment.
I just rebased this on that PR (you can see it's the first commit here) to make sure everything works with that change. I'll merge that PR first, then rebase this again so this change disappears.
| backend_converters = self.connection.ops.get_db_converters(column) | ||
| field_converters = column.field.get_db_converters(self.connection) | ||
| if backend_converters or field_converters: | ||
| converters[column.target.column] = backend_converters + field_converters | ||
| converters[name] = backend_converters + field_converters |
There was a problem hiding this comment.
Django sql compiler uses an index. Like an integer and maintain the order.
https://github.com/mongodb-forks/django/blob/mongodb-5.0.x/django/db/models/sql/compiler.py#L1483
There was a problem hiding this comment.
Right, however, MongoDB returns a document of (unordered?) fields, hence we may need to use the key lookup approach (or at least this was the reason for the original implementation in Django Mongo Engine + Python 2). Since Python 3.7 dictionaries are guaranteed to be insertion ordered. Not sure how the MongoDB server handles field ordering though (does it match the order of $project)? Could be checked but the idea is orthogonal to this PR which just builds on the existing implementation.
django_mongodb/base.py
Outdated
| mongo_aggregates = { | ||
| "exact": lambda a, b: {"$eq": [a, b]}, | ||
| "gt": lambda a, b: {"$gt": [a, b]}, | ||
| "gte": lambda a, b: {"$gte": [a, b]}, | ||
| "lt": lambda a, b: {"$lt": [a, b]}, | ||
| "lte": lambda a, b: {"$lte": [a, b]}, | ||
| } |
There was a problem hiding this comment.
Did you need the iexact, startswith, endswith equivalents here as well?
There was a problem hiding this comment.
I guess it's possible those lookups could be used as annotations, but I'm not sure the Django test suite covers it. I wouldn't worry about it unless we get a bug report.
timgraham
force-pushed
the
QuerySet.annotate
branch
4 times, most recently
from
51a65b5 to
06f34f8
Compare
|
While triaging the annotations tests, I found few more bugs which are fixed in the first two of the last three commits, so I'd appreciate it if you could review those. |
django_mongodb/base.py
Outdated
| @@ -73,6 +91,13 @@ class DatabaseWrapper(BaseDatabaseWrapper): | |||
| "regex": lambda val: re.compile(val), | |||
| "iregex": lambda val: re.compile(val, re.IGNORECASE), | |||
| } | |||
| mongo_aggregates = { | |||
There was a problem hiding this comment.
🤔 In this PR add support for annotates, so Why is mongo_aggregates the name? shouldn't it be mongo_annotates? IMO aggregates sounds like $sum, $avg, and all this kind of group by operators.
There was a problem hiding this comment.
Originally I called it mongo_functions but renamed it because https://www.mongodb.com/docs/manual/reference/operator/aggregation/lt/ describes it as "aggregation". Maybe it should be mongo_aggregations. I'm not sure the precise difference between aggregate and aggregation!
| @@ -54,7 +54,25 @@ class DatabaseWrapper(BaseDatabaseWrapper): | |||
| "TimeField": "date", | |||
| "UUIDField": "string", | |||
| } | |||
| # Django uses these operators to generate SQL queries before it generates | |||
| # MQL queries. | |||
There was a problem hiding this comment.
Can you elaborate on this? Why does SQL get generated if we're running a mongodb query?
There was a problem hiding this comment.
It has to do with how this library hooks in to the query generation process. See also:
Also fix annotating with Value().
timgraham
force-pushed
the
QuerySet.annotate
branch
from
06f34f8 to
db3b2d0
Compare
No description provided.